-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: replace OnLayoutEvent with SvgOnLayoutEvent #2738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: replace OnLayoutEvent with SvgOnLayoutEvent #2738
Conversation
4e34ac9 to
c7b0311
Compare
9541ccb to
89543b6
Compare
|
We'd love to see it merged as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of PR looks ok!
| x: Int32; | ||
| y: Int32; | ||
| width: Int32; | ||
| height: Int32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it should be Int instead of Float?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During implementation I was looking at official RN code: https://github.com/facebook/react-native/blob/main/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/OnLayoutEvent.kt
And there they have ints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also when we look at the creation of SVGOnLayoutEvent inside VirtualView we pass to this event Ints:
https://github.com/software-mansion/react-native-svg/pull/2738/files#diff-b99e5fc93019444c57df66f6135a6ff9c28d881abe126fc0adb12773b24a7193R608
1996b31 to
62b9d78
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Please make sure that it works on both archs and platforms and I think we are good with merging it.
Summary
This PR creates
SvgOnLayoutEventand replaces usage ofOnLayoutEventfor Android because React Native Android team will remove this event from their codebase in near future.Test Plan
Verified that the onLayout event emitted by Svg produces the same payload before and after this change.
Steps:
Result:
Event payload structure and values remained identical
Compatibility
Checklist